Skip to content

Add support for custom tabs in Firefox#369

Closed
hnljp wants to merge 4 commits into
openid:masterfrom
hnljp:master
Closed

Add support for custom tabs in Firefox#369
hnljp wants to merge 4 commits into
openid:masterfrom
hnljp:master

Conversation

@hnljp

@hnljp hnljp commented Jul 12, 2018

Copy link
Copy Markdown
Contributor

support for custom tabs was added to stable Firefox in version 57.0

fixes: #368
Signed-off-by: Henning Nielsen Lund henning.n.lund@jp.dk

support for custom tabs was added to stable Firefox in version 57.0

fixes: openid#368
Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
@codecov-io

codecov-io commented Jul 12, 2018

Copy link
Copy Markdown

Codecov Report

Merging #369 into master will decrease coverage by 0.17%.
The diff coverage is 65%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #369      +/-   ##
============================================
- Coverage     82.93%   82.76%   -0.18%     
  Complexity      456      456              
============================================
  Files            41       41              
  Lines          2233     2251      +18     
  Branches        217      220       +3     
============================================
+ Hits           1852     1863      +11     
- Misses          298      302       +4     
- Partials         83       86       +3
Impacted Files Coverage Δ Complexity Δ
...a/net/openid/appauth/browser/BrowserBlacklist.java 100% <ø> (ø) 4 <0> (ø) ⬇️
...rary/java/net/openid/appauth/browser/Browsers.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...penid/appauth/browser/VersionedBrowserMatcher.java 85.71% <100%> (+1.5%) 7 <0> (ø) ⬇️
...va/net/openid/appauth/browser/BrowserSelector.java 78.78% <53.33%> (-8.01%) 17 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51c5b26...57d183a. Read the comment docs.

@hnljp hnljp mentioned this pull request Jul 13, 2018
hnljp added 3 commits July 16, 2018 00:19
a possible default browser of the user is respected.

fixes: openid#372
Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
The line needed to get reduced to no more than 100 characters.

Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
Corrected indentation accoring to travis-ci.

Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
@iainmcgin

Copy link
Copy Markdown
Member

If you can separate the commits so that the firefox custom tab definitions can be pulled in separately from the rest, I can merge that piece at least. The default browser behavior needs some more discussion and investigation.

@hnljp

hnljp commented Jul 16, 2018

Copy link
Copy Markdown
Contributor Author

The ordered list is always returning Chrome as the first entry, no matter what...
This is an example, of a phone with Android 8.0, where Firefox is set as the default browser: 07-14 22:15:15.714 12422-12909/? D/BrowserSelector: resolvedActivityList: [ResolveInfo{3be23c com.android.chrome/com.google.android.apps.chrome.Main m=0x108000}, ResolveInfo{ed5cbc5 org.mozilla.firefox/.App m=0x108000}]
I do not know what their criteria is for the order, but it is not returning the default browser first.
The same is also shown on Android 7.0.

@hnljp

hnljp commented Jul 16, 2018

Copy link
Copy Markdown
Contributor Author

it could be changed if the flag used by queryIntentActivities was changed.

@hnljp

hnljp commented Jul 18, 2018

Copy link
Copy Markdown
Contributor Author

I have tried to look more into it.
The problem is that the PackageManager.MATCH_ALL flag is making the result unsorted.
https://developer.android.com/reference/android/content/pm/PackageManager#MATCH_ALL
But the problem is that removing MATCH_ALL, will make queryIntentActivities only return the default browser on Android >= 7.0.

Looking at the code generating the list returned by queryIntentActivities:
https://android.googlesource.com/platform/frameworks/base.git/+/master/services/core/java/com/android/server/pm/PackageManagerService.java#7801
That is using the list generated as "an unsorted list":
https://android.googlesource.com/platform/frameworks/base.git/+/master/services/core/java/com/android/server/pm/PackageManagerService.java#7731
That is why there is a need to get the package name of the default browser and the list using MATCH_ALL is needed, in case the default browser is not compatible with Custom Tabs.

@iainmcgin

Copy link
Copy Markdown
Member

OK, thanks for investigating further. Could you still separate this change into two pull requests, one for adding the additional constants related to Firefox, and the other for the default browser selection?

@hnljp hnljp mentioned this pull request Jul 20, 2018
@hnljp

hnljp commented Jul 20, 2018

Copy link
Copy Markdown
Contributor Author

Have made a new pull request only including the Firefox support.
Better to separate the two issues, as stated by iainmcgin
#378

hnljp added a commit to hnljp/AppAuth-Android that referenced this pull request Jul 20, 2018
a possible default browser of the user is respected.

fixes: openid#372

split from openid#369

Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
@hnljp

hnljp commented Jul 20, 2018

Copy link
Copy Markdown
Contributor Author

Have created a new pull request regarding using the default browser, if it is compatible.
#379

@hnljp hnljp closed this Jul 20, 2018
hnljp added a commit to hnljp/AppAuth-Android that referenced this pull request Jul 27, 2018
a possible default browser of the user is respected.

fixes: openid#372

split from openid#369

Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for custom tabs in Firefox

5 participants